Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement recorded::test macro for recorded tests #1926

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

heaths
Copy link
Member

@heaths heaths commented Nov 22, 2024

  • Rename typespec_derive to typespec_macros
    Will be more consistent with upcoming azure_core_macros, and may not contain just derive macros anyway. Will keep feature as "derive", though, for derive macros.

  • Remove 128-bit number functions for Cosmos

  • Implement #[recorded] attribute macro

  • Allow live-only tests with no parameters

  • Replace test_e2e feature with recorded attribute macro

Will be more consistent with upcoming azure_core_macros, and may not contain *just* derive macros anyway. Will keep feature as "derive", though, for derive macros.
@heaths
Copy link
Member Author

heaths commented Nov 22, 2024

I also had to bump the MSRV to 1.80 to use the stable LazyLock so we're not having to read the AZURE_TEST_MODE env var (or lack there of) and parsing the string for each method. We wouldn't have to do that, but 1.80 is also currently behind the latest by 4 versions (1.84).

Copy link
Member

@analogrelay analogrelay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this. My only thought, is maybe we rename this "recorded_test" or just call the attribute "test" and but it in a recorded module with the intent of being referenced as #[recorded::test]? Since it's effectively replacing the test marker entirely and the user is expected to NOT apply a standard test attribute, I think that including "test" in the name would make that clear.

@analogrelay
Copy link
Member

I also had to bump the MSRV to 1.80 to use the stable LazyLock so we're not having to read the AZURE_TEST_MODE env var (or lack there of) and parsing the string for each method.

I'm fine with bumping our MSRV, though I'll be honest I'm not sure it's necessary to cache the test mode value like this. It should work just fine though, and I like getting new Rust versions 😁

@heaths
Copy link
Member Author

heaths commented Nov 22, 2024

I'm fine with bumping our MSRV, though I'll be honest I'm not sure it's necessary to cache the test mode value like this.

Once this repo really starts churning, we'll have thousands of tests and I imagine that time will add up. But we could see. I'd rather merge this as-is and will do a subsequent PR to remove it so we have a history of it and the decisions made. I also want to do a follow-up PR (or more) to refactor how this gets exposed, which leads me into...

I like this. My only thought, is maybe we rename this "recorded_test" or just call the attribute "test" and but it in a recorded module with the intent of being referenced as #[recorded::test]? Since it's effectively replacing the test marker entirely and the user is expected to NOT apply a standard test attribute, I think that including "test" in the name would make that clear.

I was initially trying to avoid a long name like recorded_test, but I fear putting it in a module and importing that may not be as intuitive if we ended up with something like:

use azure_core_test::{recorded, TestContext, TestMode};

#[recorded::test]
async fn foo(ctx: TestContext) {
    if ctx.test_mode() == TestMode::Live {
        todo!();
    }
    todo!()
}

I mean, if someone got in the habit of just writing recorded::test, rust-analyzer should import it the first time just fine. I like the look of that better. That said, looking at some other languages for inspiration, .NET has RecordedTestAttribute (so [RecordedTest] for short) while Python has @recorded_by_proxy (and here I thought they didn't like long names).

Maybe we should have a azure_core_test::prelude. We agreed in guidelines not to in general, but didn't say we shouldn't ever. For tests, to pull everything you need to typically reference in, it makes sense. The above example just becomes:

use azure_core_test::prelude::*;

#[recorded::test]
async fn foo(ctx: TestContext) {
    if ctx.test_mode() == TestMode::Live {
        todo!();
    }
    todo!()
}

@RickWinter @JeffreyRichter what do you think?

I honestly do like the look of #[recorded::test] much better but #[recorded_test] would be fine, too.

Also makes the attribute `#[recorded::test]` which, IMO, looks a bit better.
@heaths heaths changed the title Implement recorded macro for recorded tests Implement recorded::test macro for recorded tests Nov 22, 2024
We may need to set env vars for our Windows agents to find OpenSSL, which does appear to be installed. See Azure#1746. For now, I'm removing `--all-features` from this PR since that issue is already tracking adding them separately.
@heaths heaths enabled auto-merge (squash) November 23, 2024 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants